feat: stage pod template rolls behind cluster permit by daanvinken · Pull Request #338 · valkey-io/valkey-operator

daanvinken · 2026-07-27T14:48:25Z

This PR closes #337

Summary

ValkeyCluster already updates ValkeyNode specs carefully: one node at a time, replicas before primaries, with a failover attempt before rolling a primary.

That care stopped at the ValkeyNode object. Each node rewrites its single-pod StatefulSet (or Deployment) as soon as the computed pod template differs, with no coordination across nodes. Image changes and operator upgrades that rewrite the built template could restart every pod in the cluster at once.

This PR extends the same staged idea to the pod template apply path.

Features / Behaviour Changes

Cluster-owned nodes no longer apply StatefulSet or Deployment pod template updates until the ValkeyCluster controller grants a permit (valkey.io/allow-workload-revision).
Permits are granted one node at a time (replicas first, proactive failover before a primary), sharing the same in-flight rule as Spec rolls so Spec and workload rolls cannot race.
While waiting, the node surfaces a WorkloadDrift condition and records the desired template revision for the cluster controller to grant.
Standalone ValkeyNodes (not owned by a cluster) still apply template updates immediately.
Create path is unchanged: new workloads are still created when missing.
A few API-server defaults on containers and probes are owned in the builder so leftover default noise does not keep looking like a real template change.

Implementation

workload_roll.go: hash the desired pod template, read/write permit annotations, decide grant vs wait, shared in-flight check used by both Spec and workload paths.
ValkeyNode controller: compare desired vs live template; if a cluster-owned node would roll without a matching permit, set WorkloadDrift, leave the live workload alone, and requeue.
ValkeyCluster controller: when a node reports drift (or needs a Spec update), grant at most one allow annotation per reconcile after the existing replica-first / failover ordering.
ValkeyNodeConditionWorkloadDrift on the ValkeyNode API for visibility while waiting.

Review focus: permit grant/clear transitions, the shared in-flight check when allow=* is used for Spec rolls, and that waiters do not thrash events on every requeue.

Limitations

Does not add a new user-facing CRD field for roll strategy; behaviour is automatic for cluster-owned nodes.
Permit value is either a template hash or * for Spec-driven rolls where the cluster does not know the hash yet.
Docs for the new condition / annotation are not expanded beyond the API type comment in this PR.

Testing

Unit tests for template hash stability, permit matching, drift condition helpers, and ValkeyNode envtest coverage that a cluster-owned node defers the StatefulSet template update until a permit is present, then applies and clears drift.
go test ./internal/controller/ passes.
Manual kind repro before/after: multi-node cluster, operator/template change; before fix all pods restarted together; after fix StatefulSet applies were staged and the cluster stayed healthy through the roll.

Checklist

This Pull Request is related to one issue.
Commit message explains what changed and why
Tests are added or updated.
Documentation files are updated.
I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

Operator image upgrades that change the ValkeyNode pod template were rewriting every 1-pod StatefulSet in one reconcile wave, taking the whole cluster down at once. Gate cluster-owned StatefulSet/Deployment template updates on valkey.io/allow-workload-revision (granted one node at a time by the ValkeyCluster controller, replicas first with proactive failover). Surface WorkloadDrift while waiting. Own API-server defaults so residual default noise does not re-trigger rolls. Spec and workload grants share a single in-flight permit. Standalone ValkeyNodes still apply immediately. Create path unchanged. Signed-off-by: daanvinken <daanvinken@tythus.com>

CI Check formatting failed on third-party vs local import grouping. Signed-off-by: daanvinken <daanvinken@tythus.com>

greptile-apps · 2026-07-27T15:00:27Z

Greptile Summary

This PR coordinates pod-template rollouts through per-node cluster permits.

Adds workload revision hashing, drift detection, and permit lifecycle helpers.
Stages StatefulSet and Deployment template updates one node at a time while preserving immediate creation and standalone-node updates.
Reconciles non-template workload fields when templates already match, addressing the previous review thread.
Adds API-server probe defaults, workload drift status constants, and rollout-gating tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failures remain in the fix for reconciling non-template workload fields.

T-Rex Logs

What T-Rex did

The focused envtest command timed out after 150 seconds, which matches the known control-plane startup blocker.
The after-change helper run completed with exit code 0, reporting six named tests passed in 0.017 seconds.
Asset diagnostics confirmed that Kubernetes v1.35.0 and etcd v3.6.6 binaries are present and executable.

_{Ran code and verified through T-Rex}

Important Files Changed

Filename	Overview
internal/controller/valkeynode_controller.go	Adds permit-gated workload updates and repairs the prior early-return issue by synchronizing non-template workload state when templates match.
internal/controller/valkeycluster_controller.go	Extends replica-first, failover-aware node reconciliation to grant and serialize workload rollout permits.
internal/controller/workload_roll.go	Introduces stable template comparison and shared helpers for desired revisions, permits, and in-flight rollout state.
internal/controller/valkeynode_resources.go	Mirrors additional API-server probe defaults to keep desired and persisted pod templates stable.
api/v1alpha1/valkeynode_types.go	Adds the WorkloadDrift condition and AwaitingRollPermit reason vocabulary.

Sequence Diagram

sequenceDiagram
participant CC as ValkeyCluster Controller
participant API as Kubernetes API
participant NC as ValkeyNode Controller
participant WL as StatefulSet/Deployment
CC->>API: Reconcile ValkeyNode spec
NC->>WL: Compare desired and live templates
alt Template matches
    NC->>WL: Sync labels, owner, and non-template spec
else Cluster-owned template drift
    NC->>API: Set desired revision and WorkloadDrift
    CC->>API: Grant one-node rollout permit
    NC->>WL: Apply permitted template update
    NC->>API: Clear drift after apply
    NC->>WL: Poll rollout completion
    NC->>API: Clear permit when rollout completes
else Standalone node
    NC->>WL: Apply template update immediately
end

_{Reviews (3): Last reviewed commit: "fix: satisfy lint and heal non-template ..." | Re-trigger Greptile}

Drop unused anyNodeHasInFlightWorkloadRoll. Split reconcileValkeyNode helpers so gocyclo stays under the limit. When the pod template already matches, still sync labels, owner, and Spec so non-template controller fields do not go stale. Signed-off-by: daanvinken <daanvinken@tythus.com>

jdheyburn

I did a quick scan on the review, but I don't know if I'm sold on the design yet. I would rather understand what is causing the ValkeyCluster controller to push the change out to so many nodes at once. I spent a bit of time on the original PRs to implement safe sequential rolls, so I'm interested to see what's happening.

jdheyburn · 2026-07-27T22:07:50Z

+
+// syncDeploymentWithoutRoll updates labels, owner, and Spec when the pod
+// template would not roll.
+func (r *ValkeyNodeReconciler) syncDeploymentWithoutRoll(ctx context.Context, node *valkeyiov1alpha1.ValkeyNode, dep *appsv1.Deployment, desired *appsv1.Deployment) error {


There's a lot of similarity with syncStatefulSetWithoutRoll. Are we able to DRY these? Something like syncWorkloadWithoutRoll, that might help cut the LOC down.

Fair! We can for sure

melancholictheory · 2026-07-28T09:23:14Z

this is the right extension. the sequencing the cluster does at the CR level was being undone the moment each node rewrote its own template, so gating the template apply behind the same permit closes a real gap. and it's good that maybeProactiveFailoverBeforeRoll fires for the workload path too, not just spec rolls: without that, staging alone would still restart a primary's pod as primary and drop writes. the failover-before-primary is the part that actually matters here, and you kept it.

it also composes well with #317: now that the operator owns the api-server defaults, the template only differs on genuine changes (image, real builder changes), so this stages the changes that should be staged rather than defaulting churn. the two together are the full fix for #337.

one thing to make sure is observable: the workload permit is a single cluster-wide token, so a node whose new pod never becomes Ready (bad image, failing probe) holds it indefinitely and no other node's workload roll proceeds. that's the safe behaviour, you don't want to cascade a broken rollout, but it means one stuck node wedges every other node's template roll silently. the AwaitingRollPermit condition helps, but it's worth distinguishing "waiting its turn" from "the holder is stuck" (the same updating-normally vs stuck-updating distinction from #267), so an operator can tell a slow rollout from a jammed one. a bounded wait or a stuck-holder event would do it.

(the DRY point on syncStatefulSetWithoutRoll seems right too, but that's cosmetics next to the above.)

daanvinken · 2026-07-28T12:39:17Z

Let's take the discussion on the bug itself to #337 (comment)

daanvinken added 2 commits July 27, 2026 16:33

fix: gofmt import order in workload_roll files

8371e8c

CI Check formatting failed on third-party vs local import grouping. Signed-off-by: daanvinken <daanvinken@tythus.com>

greptile-apps Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread internal/controller/valkeynode_controller.go

jdheyburn reviewed Jul 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: stage pod template rolls behind cluster permit - #338

feat: stage pod template rolls behind cluster permit#338
daanvinken wants to merge 3 commits into
valkey-io:mainfrom
daanvinken:feat/workload-roll-permit-gate

daanvinken commented Jul 27, 2026

Uh oh!

greptile-apps Bot commented Jul 27, 2026 •

edited

Loading

Greptile Summary

Confidence Score: 5/5

T-Rex Logs

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

jdheyburn left a comment

Uh oh!

jdheyburn Jul 27, 2026

Uh oh!

daanvinken Jul 28, 2026

Uh oh!

melancholictheory commented Jul 28, 2026

Uh oh!

daanvinken commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

daanvinken commented Jul 27, 2026

Summary

Features / Behaviour Changes

Implementation

Limitations

Testing

Checklist

Uh oh!

greptile-apps Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

T-Rex Logs

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

jdheyburn left a comment

Choose a reason for hiding this comment

Uh oh!

jdheyburn Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

daanvinken Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

melancholictheory commented Jul 28, 2026

Uh oh!

daanvinken commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Jul 27, 2026 •

edited

Loading